Skip to content

ZEPPELIN-1287. No need to call print to display output in PythonInterpreter #1278

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
zjffdu wants to merge 3 commits into from
Closed

ZEPPELIN-1287. No need to call print to display output in PythonInterpreter #1278

zjffdu wants to merge 3 commits into from

Conversation

@zjffdu
Copy link
Contributor

@zjffdu zjffdu commented Aug 4, 2016 •
edited
Loading

What is this PR for?

It is not necessary to call print to display output in PythonInterpreter. 2 main changes:

  • the root cause is the displayhook in bootstrap.py
  • also did some code refactoring on PythonInterpreter

What type of PR is it?

[Bug Fix]

Todos

  • - Task

What is the Jira issue?

How should this be tested?

Verify it manually

Screenshots (if appropriate)

2016-08-04_1404

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

@zjffdu zjffdu changed the title ZEPPELIN-1287. No need to call print to display output in PythonInter… ZEPPELIN-1287. No need to call print to display output in PythonInterpreter Aug 4, 2016
@zjffdu
Copy link
Contributor Author

zjffdu commented Aug 4, 2016

\cc @bzz @Leemoonsoo ready for review, not sure whether the test fail is relevant. CI is very unstable recently, we might need to fix it asap 😃.

jongyoul
jongyoul reviewed Aug 4, 2016
View reviewed changes
python/src/main/java/org/apache/zeppelin/python/PythonProcess.java Outdated
import java.io.IOException;
import java.io.OutputStreamWriter;
import java.io.InputStreamReader;
import java.io.*;
Copy link
Member

@jongyoul jongyoul Aug 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging import to "*" is not recommended. Could you please expand it?

@Leemoonsoo
Copy link
Member

Leemoonsoo commented Aug 6, 2016

Thanks for the contribution.

Tested and it works great.
Could you rebase and see if it passes CI?

@Leemoonsoo
Copy link
Member

Leemoonsoo commented Aug 7, 2016

Looks like test is not passing

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running org.apache.zeppelin.python.PythonInterpreterTest
SLF4J: Class path contains multiple SLF4J bindings.
SLF4J: Found binding in [jar:file:/home/travis/build/apache/zeppelin/zeppelin-interpreter/target/zeppelin-interpreter-0.7.0-SNAPSHOT.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: Found binding in [jar:file:/home/travis/build/apache/zeppelin/zeppelin-interpreter/target/lib/slf4j-log4j12-1.7.10.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: Found binding in [jar:file:/home/travis/.m2/repository/org/slf4j/slf4j-log4j12/1.7.10/slf4j-log4j12-1.7.10.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation.
SLF4J: Actual binding is of type [org.slf4j.impl.Log4jLoggerFactory]
log4j:WARN No appenders could be found for logger (org.apache.zeppelin.interpreter.Interpreter).
log4j:WARN Please initialize the log4j system properly.
log4j:WARN See http://logging.apache.org/log4j/1.2/faq.html#noconfig for more info.
Tests run: 5, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.629 sec <<< FAILURE! - in org.apache.zeppelin.python.PythonInterpreterTest
testInterpret(org.apache.zeppelin.python.PythonInterpreterTest)  Time elapsed: 0.015 sec  <<< FAILURE!
org.junit.ComparisonFailure: expected:<%text []print a> but was:<%text [>>>]print a>
    at org.junit.Assert.assertEquals(Assert.java:115)
    at org.junit.Assert.assertEquals(Assert.java:144)
    at org.apache.zeppelin.python.PythonInterpreterTest.testInterpret(PythonInterpreterTest.java:161)


Results :

Failed tests: 
  PythonInterpreterTest.testInterpret:161 expected:<%text []print a> but was:<%text [>>>]print a>

Tests run: 5, Failures: 1, Errors: 0, Skipped: 0

@zjffdu Could you take a look?

@zjffdu zjffdu closed this Aug 8, 2016
@zjffdu zjffdu reopened this Aug 8, 2016
@Leemoonsoo
Copy link
Member

Leemoonsoo commented Aug 8, 2016

@zjffdu Thanks for the contribution!

LGTM

@Leemoonsoo
Copy link
Member

Leemoonsoo commented Aug 9, 2016

Merge to master if there's no more discussion

@minahlee
Copy link
Member

minahlee commented Aug 10, 2016

Can we merge this into branch-0.6 too? FYI #1232 is merged to branch-0.6. It will be nice to provide same user experience both in pyspark and python interpreter

@zjffdu
Copy link
Contributor Author

zjffdu commented Aug 10, 2016

+1 for merging this into branch-0.6

1 similar comment
@Leemoonsoo
Copy link
Member

Leemoonsoo commented Aug 10, 2016

+1 for merging this into branch-0.6

@asfgit asfgit closed this in 3dec4d7 Aug 11, 2016
@minahlee minahlee mentioned this pull request Aug 11, 2016
Closed
asfgit pushed a commit that referenced this pull request Aug 12, 2016
@minahlee
[ZEPPELIN-1287][branch-0.6] No need to call print to display output i…
ad4e9e0 
…n PythonInterpreter

### What is this PR for?
Implement #1278 to merge branch-0.6

### What type of PR is it?
Bug Fix

### What is the Jira issue?
[ZEPPELIN-1287](https://issues.apache.org/jira/browse/ZEPPELIN-1287)

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no

Author: Mina Lee <[email protected]>

Closes #1320 from minahlee/branch-0.6_ZEPPELIN-1287 and squashes the following commits:

f99f8aa [Mina Lee] return result directly
ac83b14 [Mina Lee] No need to call print to display output in PythonInterpreter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zjffdu @Leemoonsoo @minahlee @jongyoul